Avoid disconnect on message timeout while waiting on monitor/signer #3721
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3721 +/- ##
==========================================
+ Coverage 89.05% 89.11% +0.06%
==========================================
Files 155 156 +1
Lines 122019 123699 +1680
Branches 122019 123699 +1680
==========================================
+ Hits 108666 110240 +1574
- Misses 10695 10778 +83
- Partials 2658 2681 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wpaulino
force-pushed
the
no-disconnect-async-monitor-signer
branch
from
2f7a61b to
d8c8195
Compare
There was a problem hiding this comment.
Not 100% confident as I am not yet up to speed on quiescence and async signers / monitor updates.
I went through another round of manual mutation testing and things looking good.
Just that one last live mutant I found - but maybe we leave this for a followup.
lightning/src/ln/channel.rs
Outdated
| /// been sent by either side but not yet irrevocably committed on both commitments because we're | ||
| /// waiting on a pending monitor update or signer request. | ||
| pub fn is_monitor_or_signer_pending_channel_update(&self) -> bool { | ||
| self.monitor_pending_revoke_and_ack || self.signer_pending_revoke_and_ack |
There was a problem hiding this comment.
nit: replacing self.monitor_pending_revoke_and_ack with false passes the test suite, but happy to address this myself in a follow-up as practice.
There was a problem hiding this comment.
Sure, feel free to follow-up. I don't think the coverage is too critical here to hold this back.
wpaulino
force-pushed
the
no-disconnect-async-monitor-signer
branch
from
d8c8195 to
49dd8d3
Compare
When sending/receiving `commitment_signed`/`revoke_and_ack`, we may expect the counterparty to follow up with one of their own in response. In some cases, they are not allowed to send it because they are actually waiting for one from us. Async monitor updates and signing requests may result in the message we need to send to the counterparty being delayed, and our disconnect logic previously did not consider that. It doesn't make sense to disconnect our counterparty when we're the ones seemingly blocking progress. This commit ensures we no longer disconnect when we're waiting on an async monitor update or signing request, unless we're negotiating quiescence. Note that while our counterparty is still able to enforce a similar disconnect logic on their side, as they have no insight into why we're not able to make progress, this commit at least helps prevent reconnection cycles with those that don't enforce one.
wpaulino
force-pushed
the
no-disconnect-async-monitor-signer
branch
from
49dd8d3 to
ede5154
Compare
When sending/receiving
commitment_signed/revoke_and_ack, we may expect the counterparty to follow up with one of their own in response. In some cases, they are not allowed to send it because they are actually waiting for one from us. Async monitor updates and signing requests may result in the message we need to send to the counterparty being delayed, and our disconnect logic previously did not consider that. It doesn't make sense to disconnect our counterparty when we're the ones seemingly blocking progress.This commit ensures we no longer disconnect when we're waiting on an async monitor update or signing request, unless we're negotiating quiescence. Note that while our counterparty is still able to enforce a similar disconnect logic on their side, as they have no insight into why we're not able to make progress, this commit at least helps prevent reconnection cycles with those that don't enforce one.